WIP: New lint: `needless_path_new` #14895

ada4a · 2025-05-25T18:48:14Z

fs::read_to_string(Path::new("foo.txt")) -> fs::read_to_string("foo.txt")

Resolves #14668

changelog: [needless_path_new]: new lint

rustbot · 2025-05-25T18:48:18Z

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

clippy_lints/src/needless_path_new.rs

samueltardieu · 2025-05-27T07:11:28Z

I think you need to check that you can use a AsRef<Path>, otherwise you might suggest removing Path::new() while you really need a &Path.

Don't forget to look at the lintcheck logs for your PR. You'll see the first hit is probably wrong, as it requires building a Option<&Path>, not a Option<dyn AsRef<Path>>.

ada4a · 2025-05-27T08:42:14Z

Don't forget to look at the lintcheck logs for your PR.

oh, didn't know about that! thank you, that's very helpful

I think you need to check that you can use a AsRef<Path>, otherwise you might suggest removing Path::new() while you really need a &Path.

You'll see the first hit [in lintcheck] is probably wrong, as it requires building a Option<&Path>, not a Option<dyn AsRef<Path>>.

I thought I already covered that with the following line:

&& implements_asref_path(*parameter)

I even have a test case for exactly this!

// no warning
fn takes_path(_: &Path) {}
takes_path(Path::new("foo"));

But now that I think about it, the check above doesn't actually work?... It just checks that whatever type parameter has itself implements AsRef<Path>, and that is the case for Path. I wonder why the test even works in that case... Maybe something to do with Path vs &Path?

I guess what I need to check instead is that parameter is impl AsRef<Path>. is_type_diagnostic_item seems to be closest to what I want, but it wouldn't include the impl part? Btw that makes me wonder if I should check for dyn AsRef<Path> as well

ada4a · 2025-05-27T08:46:58Z

But the weirdest thing is that Some(..) is not even a function/method call?... The lint is supposed to only check expressions that are ExprKind::Call or ExprKind::MethodCall -- but maybe the former actually includes anything of the form foo(bar)?...

EDIT: indeed it does. but I guess this is not actually the root cause of the problem, so we could keep it? Maybe the function/method restriction should not even be necessary, maybe we should just check that the place the expression is in requires an impl AsRef<Path>, be that place a parameter in a function call, or e.g. a RHS of an assignment, like

let _: Option<impl AsRef<Path>>: Some(Path::new("foo.txt"));

(assuming impl Trait in this position would be valid, but this is just an example)

ada4a · 2025-05-29T15:39:06Z

clippy_lints/src/needless_path_new.rs

+    let implements_asref_path = |arg| implements_trait(cx, arg, asref_def_id, &[path_ty.into()]);
+
+    if let ty::FnDef(..) | ty::FnPtr(..) = type_definition.kind() {
+        let parameters = type_definition.fn_sig(tcx).skip_binder().inputs();


I thought about this whole thing a bit more.

As I said towards the end of this comment, what we want to check is whether a particular parameter of a function is impl AsRef<Path>, i.e. find a p such that fn foo(p: impl AsRef<Path>).

But the latter is just syntax sugar for fn foo<P: AsRef<Path>>(p: P), so we are in fact looking for a parameter whose type is defined in a generic type parameter of the function.

Therefore I think we shouldn't actually calling skip_binder here. Without that, I get the following chain:

The type of type_definition.fn_sig(tcx) is PolyFnSig, which is an alias to Binder<'tcx, FnSig<'tcx>>

that one has a Binder::inputs method, which returns a Binder<'tcx, &'tcx [Ty<'tcx>]>

I can kind of iterate over that using Binder::iter, which finally gives me impl Iterator<Item=Binder<'tcx, Ty<'tcx>>>

and now I'm stuck. What I think I have achieved at this point is turn a function like fn foo<P, T>(p: P, t: T, n: u32) into something like [ p<P,T>: P, t<P, T>: T, n<P, T>: u32 ] (syntax very much pseudocode-ish). So from there I need to first "shed" the unnecessary generics, and then?... No idea to be honest

Sorry, this slipped through, indeed Zulip would be a better choice for those kind of discussion. Have you read the documentation on ParamEnv? You could get the caller bounds that the caller has to adhere to for the parameter index you're interested in, and check whether the type of the argument would meet those bounds (so that for example a bound of impl AsRef<Path> + Clone would not receive an object which implements AsRef<Path> but is not Cloneable).

Also, you will have to check that the type of the parameter you're interested in does not appear in another parameter or a clause applying to another parameter type, as for example

fn foo<P: AsRef<Path>>(x: P, y: P) { … }

must use the same type for both parameters, and you better let this untouched.

Does that help?

Oh, the whole Analysis section from the dev-guide seems to be exactly what I need! Thank you very much for this and all the other pointers, I'll read them and see what I come up with –and ask on Zulip if I don't understand something

ada4a · 2025-06-02T20:35:40Z

@samueltardieu just pinging in case the notifications slipped through. If you can't/don't want to deal with this, I'll probably ask the Zulip folks for help again?

blyxyas · 2025-06-24T20:27:21Z

Currently making the feature freeze feature and the performance project perform, I don't have the capacity right now to review this :/

r? clippy

ada4a · 2025-06-24T20:47:46Z

Oh hi! All good, I'm still very much not done with this 😅 We're slowly making progress over on Zulip: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/needless_path_new/with/525402801

blyxyas · 2025-06-24T21:39:31Z

I'm still very much not done with this

No problem, when you consider that you're in a ready-to-review state, you can mention the reviewer assigned to take a look

@rustbot author

rustbot · 2025-06-24T21:39:35Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Jarcho

r? Jarcho

I can take over the review for this.

clippy_lints/src/needless_path_new.rs

Jarcho · 2025-06-25T04:03:30Z

clippy_lints/src/needless_path_new.rs

+        let generic_args_we_can_change: Vec<_> = generic_args
+            .iter()
+            .filter_map(|g| g.as_type())
+            // if a generic is used in multiple places, we should better not touch it,
+            // since we'd need to suggest changing both parameters that using it at once,
+            // which might not be possible
+            .filter(|g| {
+                let inputs_and_output = fn_sig.inputs().iter().copied().chain([fn_sig.output()]);
+                inputs_and_output.filter(|i| i.contains(*g)).count() < 2
+            })


This is better determined once we find a call to Path::new lines up with a generic argument.

I agree, this was just a WIP thing to see if it'll even work (and have something to link to for the folks on Zulip)

clippy_lints/src/needless_path_new.rs

github-actions · 2025-07-25T06:49:42Z

Lintcheck changes for 945d5e4

Lint	Added	Removed	Changed
`clippy::needless_path_new`	3	0	0

This comment will be updated if you push new changes

rustbot · 2025-09-19T11:23:34Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

ada4a

I've certainly learned a lot in these.. 4 months, wow.

This pretty much works now, here are some things I'm personally not fully sure about.

@rustbot ready

View changes since this review

ada4a · 2025-09-19T11:33:38Z

clippy_lints/src/needless_path_new.rs

+        .filter_map(|(clause, _)| {
+            let clause = clause.kind();
+            #[expect(clippy::match_same_arms, reason = "branches have different reasons to be `None`")]
+            match clause.skip_binder() {
+                // This is what we analyze
+                // NOTE: repeats the contents of `Clause::as_trait_clause`,
+                // except we don't `Binder::rebind` as we don't care about the binder
+                ClauseKind::Trait(trait_clause) => Some(trait_clause),
+
+                // Trivially holds for `P`: `Path::new` has signature `&S -> &Path`, so any "outlives" that holds for
+                // `P` does so for `S` as well
+                ClauseKind::TypeOutlives(_) | ClauseKind::RegionOutlives(_) => None,
+
+                // Irrelevant to us: neither `AsRef` nor `Sized` have associated types
+                ClauseKind::Projection(_) => None,
+
+                // Irrelevant: we don't have anything to do with consts
+                ClauseKind::ConstArgHasType(..) | ClauseKind::ConstEvaluatable(_) | ClauseKind::HostEffect(_) => None,
+
+                // Irrelevant?: we don't deal with unstable impls
+                ClauseKind::UnstableFeature(_) => None,
+
+                ClauseKind::WellFormed(_) => None,
+            }
+        })


See last commit -- should I keep this fully explicit match, or should I just revert to `clause.as_trait_clause()? The main reason I like this is that I can enumerate why I dismiss all the clause kinds other than the trait predicates -- but maybe there is a more concise way of doing that

ada4a · 2025-09-19T11:34:18Z

clippy_lints/src/needless_path_new.rs

+            //     match cx.tcx.get_diagnostic_name(pred.def_id()) {
+            //     Some(sym::AsRef) => {
+            //         // TODO: check if it's `AsRef<Path>` in paricular
+            //     },
+            //     Some(sym::Sized) => todo!(),
+            //     _ => return false,
+            // };


I decided to forego checking for exactly AsRef and Sized, because a) imo that makes the lint more correct, and b) is just easier to explain in the lint description lol.

If you agree, I'd remove this WIP code, otherwise I'll continue on from it

should I keep this?

rustbot · 2025-10-06T21:24:27Z

☔ The latest upstream changes (possibly 5b23bd4) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned blyxyas May 25, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 25, 2025

ada4a commented May 25, 2025

View reviewed changes

clippy_lints/src/needless_path_new.rs Outdated Show resolved Hide resolved

ada4a commented May 26, 2025

View reviewed changes

clippy_lints/src/needless_path_new.rs Outdated Show resolved Hide resolved

ada4a commented May 29, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

ada4a force-pushed the needless_path_new branch from a0b0291 to d6f2bc2 Compare June 2, 2025 12:05

ada4a mentioned this pull request Jun 4, 2025

New lint: duplicate_match_guards #14971

Open

This comment has been minimized.

Sign in to view

ada4a force-pushed the needless_path_new branch from dfe7581 to c91bf12 Compare June 5, 2025 19:59

ada4a force-pushed the needless_path_new branch from c91bf12 to 8915aff Compare June 20, 2025 23:13

rustbot assigned Centri3 and unassigned blyxyas Jun 24, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 24, 2025

Jarcho requested changes Jun 25, 2025

View reviewed changes

rustbot assigned Jarcho and unassigned Centri3 Jun 25, 2025

ada4a force-pushed the needless_path_new branch 2 times, most recently from 99a3e7b to 599b3d7 Compare June 27, 2025 10:16

ada4a force-pushed the needless_path_new branch from 599b3d7 to b8a1e85 Compare July 14, 2025 17:26

ada4a force-pushed the needless_path_new branch from b8a1e85 to 84862cf Compare July 25, 2025 06:39

ada4a force-pushed the needless_path_new branch from 84862cf to d014c46 Compare August 7, 2025 21:52

ada4a force-pushed the needless_path_new branch from d014c46 to a5dd45b Compare August 22, 2025 13:37

This comment has been minimized.

Sign in to view

ada4a force-pushed the needless_path_new branch from a5dd45b to a7fb9ec Compare September 8, 2025 17:43

This comment has been minimized.

Sign in to view

ada4a force-pushed the needless_path_new branch from a7fb9ec to 2feab73 Compare September 9, 2025 16:59

ada4a force-pushed the needless_path_new branch from 2feab73 to 148fda1 Compare September 19, 2025 11:23

ada4a force-pushed the needless_path_new branch from 148fda1 to c7a07b9 Compare September 19, 2025 11:30

ada4a commented Sep 19, 2025

View reviewed changes

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 19, 2025

ada4a force-pushed the needless_path_new branch from c7a07b9 to b5660db Compare September 19, 2025 13:30

ada4a added 2 commits September 19, 2025 16:23

new lint: needless_path_new

0e88347

has_required_preds: make the Clause::as_trait_clause more explicit

945d5e4

should I keep this?

ada4a force-pushed the needless_path_new branch from b5660db to 945d5e4 Compare September 19, 2025 14:24

samueltardieu added the needs-fcp label Oct 17, 2025

WIP: New lint: needless_path_new #14895

Are you sure you want to change the base?

WIP: New lint: needless_path_new #14895

Uh oh!

Conversation

ada4a commented May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 25, 2025

Uh oh!

Uh oh!

Uh oh!

samueltardieu commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ada4a commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ada4a commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ada4a May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

ada4a Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

ada4a commented Jun 2, 2025

Uh oh!

This comment has been minimized.

blyxyas commented Jun 24, 2025

Uh oh!

ada4a commented Jun 24, 2025

Uh oh!

blyxyas commented Jun 24, 2025

Uh oh!

rustbot commented Jun 24, 2025

Uh oh!

Jarcho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jarcho Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

ada4a Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

rustbot commented Sep 19, 2025

Uh oh!

ada4a left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

ada4a Sep 19, 2025

Choose a reason for hiding this comment

Uh oh!

rustbot commented Oct 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

WIP: New lint: `needless_path_new` #14895

WIP: New lint: `needless_path_new` #14895

ada4a commented May 25, 2025 •

edited

Loading

samueltardieu commented May 27, 2025 •

edited

Loading

ada4a commented May 27, 2025 •

edited

Loading

ada4a commented May 27, 2025 •

edited

Loading

ada4a May 29, 2025 •

edited

Loading

github-actions bot commented Jul 25, 2025 •

edited

Loading

ada4a left a comment •

edited

Loading